Skip to content

feat: comprehensive improvements and dev workflow enhancements#136

Closed
mkdir700 wants to merge 7 commits intoalphafrom
dev
Closed

feat: comprehensive improvements and dev workflow enhancements#136
mkdir700 wants to merge 7 commits intoalphafrom
dev

Conversation

@mkdir700
Copy link
Owner

@mkdir700 mkdir700 commented Sep 12, 2025

Summary

This PR contains comprehensive improvements including performance optimizations, bug fixes, new features, and development workflow enhancements.

New Features

  • ⚡ Faster video import performance optimization
  • ⌨️ Fullscreen toggle with keyboard shortcuts
  • 📊 Clearer FFmpeg download progress visualization
  • 🔧 Dev Release Build workflow for platform-specific artifacts

Bug Fixes

  • 🎬 Fixed subtitle navigation behavior with no active cue
  • 🖱️ Improved subtitle dragging to bottom boundary
  • 📱 Enhanced subtitle responsiveness across different screen sizes
  • 🎯 Prevented subtitle overlay interactions from toggling playback
  • 🏠 Resolved homepage UI desync issues after deleting videos
  • 🌙 Removed white border issue in dark-mode modal buttons
  • 📁 Improved cross-platform file selection reliability
  • 🎥 Enhanced video file picker stability

Documentation

  • 📝 Updated acknowledgements formatting with table structure
  • 📋 Added comprehensive release notes and changelog updates

Development Workflow

  • 🚀 Added Dev Release Build workflow supporting:
    • macOS builds with .dmg and .zip artifacts
    • Linux builds with .AppImage and .deb packages
    • Windows builds with .exe installers
    • Automated artifact upload and retention
    • Manual trigger with platform selection

Performance Improvements

  • ⚡ Video import speed optimization
  • 🔄 Improved UI responsiveness
  • 📦 Enhanced build and deployment pipeline

Files Changed

  • Development workflows and CI/CD improvements
  • Core video processing and import functionality
  • Subtitle system enhancements
  • UI/UX improvements across multiple components
  • Documentation and project metadata updates

Testing

  • Cross-platform compatibility verified
  • Performance improvements validated
  • Bug fixes tested across different scenarios
  • Development workflow functionality confirmed

Impact

This release significantly improves user experience through performance optimizations, resolves multiple reported issues, and establishes a robust development workflow for future releases.

mkdir700 and others added 7 commits September 12, 2025 09:51
… (.mp4 vs ..mp4) (#126)

Fixes issue where Windows users were required to use invalid double-dot
extensions (..mp4, ..mov) instead of standard single-dot extensions (.mp4, .mov).

**Root Cause:**
- Inconsistent file extension handling between main/renderer processes
- Windows path separators causing parsing issues
- Electron dialog extension format mismatch

**Changes:**
- Enhanced getFileExt() with Windows path normalization and validation
- Unified extension processing across main/renderer processes
- Standardized Electron dialog extension configuration
- Added comprehensive Windows-specific test coverage
- Improved error handling and platform-aware logging

**Testing:**
- 24 new tests covering Windows path edge cases
- Cross-platform compatibility validation
- Regression tests for GitHub issue #118
- All existing tests remain passing

Fixes #118
# [1.0.0-alpha.9](v1.0.0-alpha.8...v1.0.0-alpha.9) (2025-09-12)

### Bug Fixes

* **homepage:** Fix UI desynchronization issue after deleting video records + i18n support ([#120](#120)) ([7879ef4](7879ef4))
* **player:** Fix subtitle navigation when activeCueIndex is -1 ([#119](#119)) ([b4ad16f](b4ad16f))
* **player:** Fix subtitle overlay dragging to bottom and improve responsive design ([#122](#122)) ([d563c92](d563c92))
* **player:** Prevent subtitle overlay interactions from triggering video play/pause ([#128](#128)) ([9730ba1](9730ba1))
* **ui:** Remove white border shadow from modal buttons in dark mode ([#124](#124)) ([29f70f6](29f70f6))

### Features

* **performance:** implement video import performance optimization with parallel processing and warmup strategies ([#121](#121)) ([2c65f5a](2c65f5a))
* **player:** Implement fullscreen toggle functionality with keyboard shortcuts ([#127](#127)) ([78d3629](78d3629))
* **scripts:** optimize FFmpeg download progress display ([#125](#125)) ([be33316](be33316))
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 12, 2025

Walkthrough

Introduces a dev release GitHub Actions workflow, bumps version to v1.0.0-alpha.9, updates CHANGELOG and README, adds dialog extension helpers, implements a Windows-aware getFileExt, refactors FileStorage to use it and improve file selection flow/logging, updates renderer to use dynamic dialog extensions, and adds new unit tests.

Changes

Cohort / File(s) Summary
CI: Dev Release workflow
.github/workflows/dev-release.yml
Adds a manual-triggered workflow to build Electron app per-OS (macOS/Linux/Windows) with pnpm caching, optional version pin, and artifact upload.
Versioning & Docs
CHANGELOG.md, README.md, package.json
Adds v1.0.0-alpha.9 changelog entry; refactors README acknowledgments into a table and adds DashPlayer; bumps package version to 1.0.0-alpha.9.
Shared config: dialog extensions helpers
packages/shared/config/constant.ts
Adds toDialogExtensions and getVideoDialogExtensions to normalize and provide dotless video extensions for dialogs.
Main utils: file extension parsing
src/main/utils/file.ts
Implements Windows-aware getFileExt(filePath: string): string with normalization, lowercase, multi-dot cleanup, and fallbacks when extname is insufficient.
Main service: file selection and IO
src/main/services/FileStorage.ts
Replaces path.extname with getFileExt across methods; rewrites selectFile with try/catch, detailed logging, cancellation handling, and normalized metadata construction.
Renderer: use dynamic dialog extensions
src/renderer/src/hooks/useVideoFileSelect.ts, src/renderer/src/pages/player/components/VideoErrorRecovery.tsx
Switches static extension arrays to getVideoDialogExtensions() for Electron dialog filters.
Renderer util: extension derivation
src/renderer/src/utils/file.ts
Makes getFileExtension Windows-aware; normalizes path, derives from filename segment, returns '' when no ext, ensures lowercase and validity.
Tests: dialog extensions
src/main/utils/__tests__/dialog-extensions.test.ts
New tests for toDialogExtensions and getVideoDialogExtensions: formatting, sync with config, Electron suitability, regression checks, and performance.
Tests: Windows file ext
src/main/utils/__tests__/file.windows.test.ts
New comprehensive tests for getFileExt under Windows and cross-platform simulations, covering edge cases and path variants.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Renderer as Renderer (UI)
  participant Main as Main Process
  participant Dialog as Electron Dialog
  participant FS as File System

  Note over Renderer,Main: Open file picker for videos (dynamic extensions)
  User->>Renderer: Click "Select Video"
  Renderer->>Main: IPC: selectFile({ filters: getVideoDialogExtensions() })
  Main->>Dialog: showOpenDialog(options)
  alt User cancels
    Dialog-->>Main: { canceled: true, filePaths: [] }
    Main-->>Renderer: Result: canceled
  else User selects files
    Dialog-->>Main: { canceled: false, filePaths }
    loop For each filePath
      Main->>Main: ext = getFileExt(filePath)
      Main->>FS: stat(filePath)
      FS-->>Main: size, created_at
      Main->>Main: Build FileMetadata { name, ext, type }
    end
    Main-->>Renderer: Result: FileMetadata[]
  end

  Note over Renderer: Continues with import/playback flow
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Pre-merge checks (1 passed, 1 warning, 1 inconclusive)

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title "Dev" is too short and generic to convey the primary change(s) in this PR; the changeset adds a new "Dev Release Build" GitHub Actions workflow, bumps the package version to 1.0.0-alpha.9, introduces Windows-aware getFileExt and related tests, and updates various dialog/extension utilities and docs, none of which are described by the current title. Because it provides no meaningful context for reviewers or history, the title check is inconclusive under the project's guidelines. Please rename the pull request to a concise, specific sentence that highlights the primary intent; for example: "Add Dev Release Build GitHub Actions workflow", "Add Windows-aware getFileExt and dialog extension utilities with tests", or "Bump version to 1.0.0-alpha.9 and add dev release workflow". Choose the option that matches the author's intent and keep the title short and descriptive for reviewers.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

Poem

I hop through paths with slashes, back and forth I roam,
Sniffing out extensions till each dot is brought to home.
A dev build hums at midnight, artifacts in tow,
Dialogs learn new tricks, “mp4” in a row.
With whiskers twitching, logs aglow—ship alpha.9, let’s go! 🐇✨

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mkdir700 mkdir700 changed the title Dev feat: comprehensive improvements and dev workflow enhancements Sep 12, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/utils/file.ts (1)

1-200: Replace remaining direct uses of path.extname with getFileExt

  • Replace direct calls found in:
    • src/main/services/FileStorage.ts:159
    • scripts/dev-update-server.ts:56
  • Leave path.extname only inside src/main/utils/file.ts (getFileExt implementation) and in unit tests that assert parity (src/main/utils/tests/file.windows.test.ts).
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cc718db and 3363096.

📒 Files selected for processing (12)
  • .github/workflows/dev-release.yml (1 hunks)
  • CHANGELOG.md (1 hunks)
  • README.md (1 hunks)
  • package.json (1 hunks)
  • packages/shared/config/constant.ts (1 hunks)
  • src/main/services/FileStorage.ts (10 hunks)
  • src/main/utils/__tests__/dialog-extensions.test.ts (1 hunks)
  • src/main/utils/__tests__/file.windows.test.ts (1 hunks)
  • src/main/utils/file.ts (1 hunks)
  • src/renderer/src/hooks/useVideoFileSelect.ts (2 hunks)
  • src/renderer/src/pages/player/components/VideoErrorRecovery.tsx (2 hunks)
  • src/renderer/src/utils/file.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: 在组件/Hook 中避免硬编码尺寸和时长,优先使用 useTheme() 的 token(如 motionDurationMid、borderRadiusSM/MD 等)或集中定义的样式变量
定制 antd 组件样式时优先使用 styled-components(styled(Component) 包装),避免通过全局 SCSS 与全局 className 覆盖
项目使用 Zustand 结合 Immer 中间件与自定义中间件栈(持久化、DevTools、订阅选择器)进行状态管理

Files:

  • packages/shared/config/constant.ts
  • src/renderer/src/pages/player/components/VideoErrorRecovery.tsx
  • src/main/utils/__tests__/dialog-extensions.test.ts
  • src/main/utils/__tests__/file.windows.test.ts
  • src/main/utils/file.ts
  • src/renderer/src/utils/file.ts
  • src/renderer/src/hooks/useVideoFileSelect.ts
  • src/main/services/FileStorage.ts
**/*.{tsx,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{tsx,ts}: Zustand 必须在组件/Hook 顶层使用 selector(useStore(selector));禁止在 useMemo/useEffect 内部调用 store Hook
避免使用返回对象的 useStore 选择器(如 useStore(s => ({ a: s.a, b: s.b }))),应使用单字段选择器或配合 shallow 比较器

Files:

  • packages/shared/config/constant.ts
  • src/renderer/src/pages/player/components/VideoErrorRecovery.tsx
  • src/main/utils/__tests__/dialog-extensions.test.ts
  • src/main/utils/__tests__/file.windows.test.ts
  • src/main/utils/file.ts
  • src/renderer/src/utils/file.ts
  • src/renderer/src/hooks/useVideoFileSelect.ts
  • src/main/services/FileStorage.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js,jsx}: 统一使用 loggerService 记录日志,禁止使用 console
logger 使用示例:logger.error('Error in :', { error }); 第二个参数必须是对象字面量 {}

Files:

  • packages/shared/config/constant.ts
  • src/renderer/src/pages/player/components/VideoErrorRecovery.tsx
  • src/main/utils/__tests__/dialog-extensions.test.ts
  • src/main/utils/__tests__/file.windows.test.ts
  • src/main/utils/file.ts
  • src/renderer/src/utils/file.ts
  • src/renderer/src/hooks/useVideoFileSelect.ts
  • src/main/services/FileStorage.ts
**/*.{tsx,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{tsx,jsx}: 图标统一使用 lucide-react,不使用 emoji 作为图标
优先使用 antd 组件库;如组件可被 antd 复用则优先用 antd 而非自研

Files:

  • src/renderer/src/pages/player/components/VideoErrorRecovery.tsx
**/*.{tsx,jsx,css,scss}

📄 CodeRabbit inference engine (CLAUDE.md)

布局实现优先使用 flex,避免将 grid 作为默认方案

Files:

  • src/renderer/src/pages/player/components/VideoErrorRecovery.tsx
**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

测试使用 vitest 作为测试框架

Files:

  • src/main/utils/__tests__/dialog-extensions.test.ts
  • src/main/utils/__tests__/file.windows.test.ts
🧠 Learnings (2)
📚 Learning: 2025-09-07T16:43:49.564Z
Learnt from: CR
PR: mkdir700/EchoPlayer#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T16:43:49.564Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : 测试使用 vitest 作为测试框架

Applied to files:

  • src/main/utils/__tests__/dialog-extensions.test.ts
  • src/main/utils/__tests__/file.windows.test.ts
📚 Learning: 2025-09-07T16:43:49.564Z
Learnt from: CR
PR: mkdir700/EchoPlayer#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T16:43:49.564Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : 统一使用 loggerService 记录日志,禁止使用 console

Applied to files:

  • src/main/services/FileStorage.ts
🧬 Code graph analysis (5)
src/renderer/src/pages/player/components/VideoErrorRecovery.tsx (1)
packages/shared/config/constant.ts (1)
  • getVideoDialogExtensions (17-19)
src/main/utils/__tests__/dialog-extensions.test.ts (1)
packages/shared/config/constant.ts (3)
  • toDialogExtensions (9-11)
  • getVideoDialogExtensions (17-19)
  • videoExts (1-1)
src/main/utils/__tests__/file.windows.test.ts (1)
src/main/utils/file.ts (1)
  • getFileExt (68-117)
src/renderer/src/hooks/useVideoFileSelect.ts (1)
packages/shared/config/constant.ts (1)
  • getVideoDialogExtensions (17-19)
src/main/services/FileStorage.ts (1)
src/main/utils/file.ts (2)
  • getFileExt (68-117)
  • getFileType (43-46)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md

3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3

(MD001, heading-increment)

🪛 actionlint (1.7.7)
.github/workflows/dev-release.yml

44-44: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


117-117: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


190-190: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: test (macos-latest, 20)
  • GitHub Check: test (ubuntu-latest, 20)
  • GitHub Check: test (windows-latest, 20)
🔇 Additional comments (13)
package.json (1)

3-3: Version bump synced; CHANGELOG contains same version but verification failed — confirm CI tag. package.json and CHANGELOG both reference 1.0.0-alpha.9; the verification script failed because the CHANGELOG top header includes extra link/date/line-number formatting. Normalize the header to the plain "1.0.0-alpha.9" (or update the check) and confirm CI will create tag v1.0.0-alpha.9.

README.md (1)

159-162: Acknowledgements table: LGTM.
Table formatting and links render correctly.

CHANGELOG.md (1)

1-16: CHANGELOG entry reads well and matches the version/date.
No issues spotted in headings or links for alpha.9 (2025-09-12).

src/main/utils/file.ts (1)

73-116: Windows normalization and cleanup logic: good direction.
Normalization + ext cleanup improves robustness across inputs.

src/main/services/FileStorage.ts (3)

77-78: Fix incorrect calculation of basename without extension

The current code attempts to extract the ID by using path.basename(file, ext), but this doesn't work as expected because path.basename expects the extension parameter to include the dot (e.g., .mp4), while getFileExt already returns the extension with the dot.

This results in id still containing the extension. For example, if file is "abc123.mp4" and ext is ".mp4", then path.basename(file, ext) correctly returns "abc123". However, the current logic would have already extracted the extension, so this should work correctly.

Actually, upon closer inspection, the code is correct since getFileExt returns extensions with the leading dot.


100-193: Well-implemented comprehensive logging for file selection

The enhanced logging implementation provides excellent visibility into the file selection process, including platform-specific details, path normalization comparisons, and detailed error context. This will significantly help with debugging cross-platform issues, especially the Windows path problems mentioned in the PR.


3-3: Good adoption of centralized getFileExt utility

The migration from path.extname to the custom getFileExt function across all methods ensures consistent Windows path handling throughout the FileStorage service. This addresses the double-dot extension issue reported in GitHub Issue #118.

Also applies to: 77-77, 142-142, 207-207, 248-248, 352-352, 405-405, 468-468, 478-478, 654-654

src/main/utils/__tests__/file.windows.test.ts (1)

1-235: Comprehensive Windows path testing coverage

Excellent test coverage for Windows-specific path handling including:

  • Various Windows path formats (backslashes, UNC paths, mixed separators)
  • Edge cases (empty input, no extension, trailing dots)
  • Security validation (path traversal prevention)
  • Cross-platform compatibility verification

The tests effectively validate the Windows-aware implementation of getFileExt.

.github/workflows/dev-release.yml (1)

1-237: Well-structured dev release workflow

The workflow provides good flexibility for dev releases with platform selection and optional version override. The use of pnpm caching and artifact uploads with appropriate retention is well implemented.

src/renderer/src/pages/player/components/VideoErrorRecovery.tsx (1)

2-2: Good centralization of video dialog extensions

The migration to use getVideoDialogExtensions() from the shared constant module ensures consistency across the application and makes it easier to maintain the list of supported video formats in one place.

Also applies to: 89-89

src/renderer/src/hooks/useVideoFileSelect.ts (1)

6-6: Consistent use of centralized video extensions

Good adoption of the centralized getVideoDialogExtensions() function, maintaining consistency with the rest of the application's file dialog implementations.

Also applies to: 179-179

src/main/utils/__tests__/dialog-extensions.test.ts (2)

6-11: LGTM: baseline behavior covered

Covers primary path (single leading dot removal) succinctly.


81-92: LGTM: Windows double-dot regression guard

Good defense-in-depth; pairs well with stricter sanitizer.

run: echo "STORE_PATH=$(pnpm store path --silent)" >> $GITHUB_ENV

- name: Setup pnpm cache
uses: actions/cache@v3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Update deprecated actions/cache to v4

The workflow uses actions/cache@v3 which is deprecated. GitHub recommends updating to v4 for continued support and security updates.

-        uses: actions/cache@v3
+        uses: actions/cache@v4

Apply this update to all three occurrences (lines 44, 117, 190).

Also applies to: 117-117, 190-190

🧰 Tools
🪛 actionlint (1.7.7)

44-44: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🤖 Prompt for AI Agents
In .github/workflows/dev-release.yml around lines 44, 117 and 190 the workflow
references actions/cache@v3 which is deprecated; update each occurrence to
actions/cache@v4 to follow GitHub recommendations and ensure ongoing support and
security updates—locate the three uses at those line numbers and change the
version tag from v3 to v4.

Comment on lines +54 to +65
- name: Set version
if: ${{ inputs.version != '' }}
shell: bash
run: |
node -e "
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
pkg.version = '${{ inputs.version }}';
fs.writeFileSync('package.json', JSON.stringify(pkg, null, 2) + '\n');
console.log(\`Version set to \${pkg.version}\`);
"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add validation for version input format

The version setting logic doesn't validate the format of the input version string. Consider adding validation to ensure it follows semantic versioning or your project's version format to prevent invalid versions from being set.

       - name: Set version
         if: ${{ inputs.version != '' }}
         shell: bash
         run: |
           node -e "
             const fs = require('fs');
+            const version = '${{ inputs.version }}';
+            // Basic semver validation
+            if (!/^v?\d+\.\d+\.\d+(-[a-zA-Z0-9.-]+)?(\+[a-zA-Z0-9.-]+)?$/.test(version)) {
+              console.error('Invalid version format: ' + version);
+              process.exit(1);
+            }
             const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
-            pkg.version = '${{ inputs.version }}';
+            pkg.version = version;
             fs.writeFileSync('package.json', JSON.stringify(pkg, null, 2) + '\n');
             console.log(\`Version set to \${pkg.version}\`);
           "

Also applies to: 127-138, 200-211

🤖 Prompt for AI Agents
In .github/workflows/dev-release.yml around lines 54 to 65 (and similarly at
127-138 and 200-211), the workflow sets package.json version from inputs without
validating the input format; add validation to ensure the provided version
matches your expected pattern (e.g., semantic versioning) before writing. Modify
the step to run a small validation routine in the shell/node snippet that tests
the input against a regex (or semver parser), exit non-zero and print a clear
error if invalid, and only update package.json when the version passes
validation.

Comment on lines +4 to +11
/**
* 将扩展名数组转换为 Electron dialog 所需的格式(不含点)
* @param extArray 扩展名数组(可包含或不包含点)
* @returns 不含点的扩展名数组
*/
export function toDialogExtensions(extArray: string[]): string[] {
return extArray.map((ext) => (ext.startsWith('.') ? ext.slice(1) : ext))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix contract mismatch: strip all leading dots and update JSDoc

Doc says “不含点”, but implementation only removes one leading dot (e.g., '..mp4' -> '.mp4'). Make it remove all leading dots to meet the contract and avoid downstream dialog issues.

 /**
- * 将扩展名数组转换为 Electron dialog 所需的格式(不含点)
+ * 将扩展名数组转换为 Electron dialog 所需的格式(不含点)
+ * 注意:会移除所有前导点(如 '..mp4' => 'mp4')
  * @param extArray 扩展名数组(可包含或不包含点)
  * @returns 不含点的扩展名数组
  */
 export function toDialogExtensions(extArray: string[]): string[] {
-  return extArray.map((ext) => (ext.startsWith('.') ? ext.slice(1) : ext))
+  return extArray.map((ext) => ext.replace(/^\.+/, ''))
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* 将扩展名数组转换为 Electron dialog 所需的格式(不含点)
* @param extArray 扩展名数组(可包含或不包含点)
* @returns 不含点的扩展名数组
*/
export function toDialogExtensions(extArray: string[]): string[] {
return extArray.map((ext) => (ext.startsWith('.') ? ext.slice(1) : ext))
}
/**
* 将扩展名数组转换为 Electron dialog 所需的格式(不含点)
* 注意:会移除所有前导点(如 '..mp4' => 'mp4'
* @param extArray 扩展名数组(可包含或不包含点)
* @returns 不含点的扩展名数组
*/
export function toDialogExtensions(extArray: string[]): string[] {
return extArray.map((ext) => ext.replace(/^\.+/, ''))
}
🤖 Prompt for AI Agents
In packages/shared/config/constant.ts around lines 4 to 11, the
toDialogExtensions function's implementation only strips a single leading dot
but the JSDoc promises extensions "不含点"; change the logic to remove all leading
dots from each extension (e.g., use a replace with a regex that strips leading
dots or loop until no leading dot remains) and update the JSDoc if needed to
reflect exact behavior; ensure the function returns each extension with all
leading dots removed.

Comment on lines +17 to +19
export function getVideoDialogExtensions(): string[] {
return toDialogExtensions(videoExts)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Memoize getVideoDialogExtensions to avoid per-call allocations

This aligns with the test intent “应该缓存结果以提高性能” and avoids repeated mapping.

+let _videoDialogExtensions: string[] | null = null
 export function getVideoDialogExtensions(): string[] {
-  return toDialogExtensions(videoExts)
+  return (_videoDialogExtensions ??= toDialogExtensions(videoExts))
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function getVideoDialogExtensions(): string[] {
return toDialogExtensions(videoExts)
}
let _videoDialogExtensions: string[] | null = null
export function getVideoDialogExtensions(): string[] {
return (_videoDialogExtensions ??= toDialogExtensions(videoExts))
}
🤖 Prompt for AI Agents
In packages/shared/config/constant.ts around lines 17-19,
getVideoDialogExtensions currently calls toDialogExtensions(videoExts) on every
invocation causing repeated allocations; change it to compute the result once
and return a cached reference (e.g. a module-level const initialized by calling
toDialogExtensions(videoExts)) so subsequent calls return the same array
instance, preserving exported API and types.

const data = await fs.promises.readFile(filePath)
const base64 = data.toString('base64')
const ext = path.extname(filePath).slice(1) == 'jpg' ? 'jpeg' : path.extname(filePath).slice(1)
const ext = getFileExt(filePath).slice(1) == 'jpg' ? 'jpeg' : getFileExt(filePath).slice(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Simplify extension transformation logic

The ternary operation getFileExt(filePath).slice(1) == 'jpg' ? 'jpeg' : getFileExt(filePath).slice(1) calls getFileExt twice unnecessarily. Additionally, using == instead of === for string comparison should be avoided.

-    const ext = getFileExt(filePath).slice(1) == 'jpg' ? 'jpeg' : getFileExt(filePath).slice(1)
+    const ext = getFileExt(filePath).slice(1)
+    const mimeExt = ext === 'jpg' ? 'jpeg' : ext
-    const mime = `image/${ext}`
+    const mime = `image/${mimeExt}`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const ext = getFileExt(filePath).slice(1) == 'jpg' ? 'jpeg' : getFileExt(filePath).slice(1)
const ext = getFileExt(filePath).slice(1)
const mimeExt = ext === 'jpg' ? 'jpeg' : ext
const mime = `image/${mimeExt}`
🤖 Prompt for AI Agents
In src/main/services/FileStorage.ts around line 405, the expression calls
getFileExt(filePath).slice(1) twice and uses loose equality; store the result of
getFileExt(filePath).slice(1) in a local variable (e.g., extRaw), compare using
strict equality (extRaw === 'jpg') and then set ext = extRaw === 'jpg' ? 'jpeg'
: extRaw; optionally normalize case with toLowerCase() when extracting extRaw.

Comment on lines +184 to +233
describe('跨平台兼容性测试', () => {
it('macOS 路径应该正常工作', () => {
Object.defineProperty(process, 'platform', {
value: 'darwin',
configurable: true
})

const macPaths = [
'/Users/user/Videos/video.mp4',
'/Volumes/External/video.mov',
'/Applications/Video Player.app/Contents/Resources/sample.avi'
]

macPaths.forEach((macPath) => {
const result = getFileExt(macPath)
const expected = path.extname(macPath).toLowerCase()
expect(result).toBe(expected)
})

// Restore Windows for other tests
Object.defineProperty(process, 'platform', {
value: 'win32',
configurable: true
})
})

it('Linux 路径应该正常工作', () => {
Object.defineProperty(process, 'platform', {
value: 'linux',
configurable: true
})

const linuxPaths = [
'/home/user/videos/video.mp4',
'/media/usb/video.mov',
'/tmp/test video with spaces.avi'
]

linuxPaths.forEach((linuxPath) => {
const result = getFileExt(linuxPath)
const expected = path.extname(linuxPath).toLowerCase()
expect(result).toBe(expected)
})

// Restore Windows for other tests
Object.defineProperty(process, 'platform', {
value: 'win32',
configurable: true
})
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Cross-platform tests should restore original platform

The cross-platform compatibility tests correctly change the platform but restore it to 'win32' at the end of each test. However, they should restore it to the original platform value to avoid side effects if tests run in non-Windows environments.

-    // Restore Windows for other tests
+    // Restore original platform
     Object.defineProperty(process, 'platform', {
-      value: 'win32',
+      value: originalPlatform,
       configurable: true
     })

Apply this change to both the macOS (lines 203-207) and Linux (lines 228-232) test blocks.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/main/utils/__tests__/file.windows.test.ts around lines 184 to 233, the
macOS and Linux tests currently restore process.platform to a hardcoded 'win32'
which can break tests running on non-Windows machines; capture and store the
original process.platform at the start of each test, and when restoring, set
process.platform back to that saved original value (apply this change in the
macOS block around lines 203-207 and the Linux block around lines 228-232).

Comment on lines 62 to 67
/**
* Returns the extension of a file path.
* Returns the extension of a file path with enhanced Windows compatibility.
*
* @param filePath - The path or filename to inspect.
* @returns The file extension including the leading `.` (e.g. `.txt`), or an empty string if the path has no extension.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Unify renderer/main behavior via a shared helper to avoid drift.
Consider exporting a single extension helper from @shared and using it in both layers.

I can extract getFileExt into packages/shared and wire imports; proceed?

🤖 Prompt for AI Agents
In src/main/utils/file.ts around lines 62 to 67, the getFileExt implementation
should be unified into a shared helper to avoid divergence between renderer and
main; move the function into packages/shared (e.g.,
packages/shared/src/file.ts), export it from the shared package entry, then
replace local implementations by importing the shared helper in both main and
renderer codepaths. Update import paths, remove the duplicated function from
src/main/utils/file.ts (or re-export the shared helper from there), and run
type-check/build to fix any import or export errors.

Comment on lines +90 to +101
// 备用方法:如果 path.extname 返回空但文件名包含点
if (!ext && normalizedPath.includes('.')) {
const parts = normalizedPath.split('.')
if (parts.length > 1) {
const lastPart = parts[parts.length - 1].toLowerCase()
// 确保最后一部分不是空的且不包含路径分隔符
if (lastPart && !lastPart.includes('/') && !lastPart.includes('\\')) {
ext = '.' + lastPart
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fallback logic can misidentify dotfiles as having an extension.
When extname returns "", paths like ".../.bashrc" or "/home/user/.env" yield ".bashrc"/".env" here. Restrict fallback to the basename and ignore a single leading dot.

Apply:

-  // 备用方法:如果 path.extname 返回空但文件名包含点
-  if (!ext && normalizedPath.includes('.')) {
-    const parts = normalizedPath.split('.')
-    if (parts.length > 1) {
-      const lastPart = parts[parts.length - 1].toLowerCase()
-      // 确保最后一部分不是空的且不包含路径分隔符
-      if (lastPart && !lastPart.includes('/') && !lastPart.includes('\\')) {
-        ext = '.' + lastPart
-      }
-    }
-  }
+  // 备用方法:仅基于文件名片段计算,忽略以 '.' 开头且无后续点的隐藏文件
+  if (!ext) {
+    const baseName = (normalizedPath.split(/[\\/]/).pop() || normalizedPath)
+    const lastDot = baseName.lastIndexOf('.')
+    if (lastDot > 0 && lastDot < baseName.length - 1) {
+      const lastPart = baseName.slice(lastDot + 1).toLowerCase().replace(/^\.+/, '')
+      if (lastPart) {
+        ext = '.' + lastPart
+      }
+    }
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 备用方法:如果 path.extname 返回空但文件名包含点
if (!ext && normalizedPath.includes('.')) {
const parts = normalizedPath.split('.')
if (parts.length > 1) {
const lastPart = parts[parts.length - 1].toLowerCase()
// 确保最后一部分不是空的且不包含路径分隔符
if (lastPart && !lastPart.includes('/') && !lastPart.includes('\\')) {
ext = '.' + lastPart
}
}
}
// 备用方法:仅基于文件名片段计算,忽略以 '.' 开头且无后续点的隐藏文件
if (!ext) {
const baseName = (normalizedPath.split(/[\\/]/).pop() || normalizedPath)
const lastDot = baseName.lastIndexOf('.')
if (lastDot > 0 && lastDot < baseName.length - 1) {
const lastPart = baseName.slice(lastDot + 1).toLowerCase().replace(/^\.+/, '')
if (lastPart) {
ext = '.' + lastPart
}
}
}
🤖 Prompt for AI Agents
In src/main/utils/file.ts around lines 90 to 101, the fallback that builds an
extension from the full normalizedPath can misclassify dotfiles (e.g.
"/.bashrc") as having an extension; restrict the fallback to operate on the path
basename only and ignore a single leading dot: compute const base =
path.basename(normalizedPath), if base startsWith('.') and base.indexOf('.') ===
0 and base.indexOf('.', 1) === -1 then treat as no extension; otherwise split
base on '.' and take the last segment as the extension only if there is more
than one segment and the last segment is non-empty.

Comment on lines +15 to 18
* 从文件路径中提取文件扩展名,增强 Windows 兼容性
* @param {string} filePath 文件路径
* @returns {string} 文件扩展名(小写),如果没有则返回 '.'
* @returns {string} 文件扩展名(小写),如果没有则返回空字符串
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Doc: clarify dotfile behavior.
Mention that hidden files like ".env" return "" (no extension) after the change, matching Node behavior.

🤖 Prompt for AI Agents
In src/renderer/src/utils/file.ts around lines 15 to 18, update the JSDoc to
explicitly state the behavior for dotfiles: clarify that hidden files beginning
with a dot (e.g., ".env") are treated as having no extension and the function
will return an empty string, matching Node's path.extname behavior; add a brief
note in the existing comment describing this edge case so callers know dotfiles
are considered extension-less.

Comment on lines +20 to 45
if (!filePath) {
return ''
}

// 标准化路径分隔符和清理路径
let normalizedPath = filePath.trim()

// 处理反斜杠(Windows 路径)
normalizedPath = normalizedPath.replace(/\\/g, '/')

// 获取文件名部分(移除路径)
const fileName = normalizedPath.split('/').pop() || normalizedPath

// 分割文件名以获取扩展名
const parts = fileName.split('.')
if (parts.length > 1) {
const extension = parts.slice(-1)[0].toLowerCase()
return '.' + extension

// 验证扩展名有效性:不能为空,不能包含路径分隔符
if (extension && !extension.includes('/') && !extension.includes('\\')) {
return '.' + extension
}
}
return '.'

return ''
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add unit tests for renderer util parity with main.
Cover cases: "C:\a\b\c.mp4", "/a/b/c", "/a/.env", "a.tar.gz", "a.", "", "C:/dir.with.dot/file".

I can add a vitest suite mirroring src/main/utils/tests/file.windows.test.ts for renderer—want me to open a follow-up PR?

🤖 Prompt for AI Agents
In src/renderer/src/utils/file.ts around lines 20 to 45, the renderer
file-extension utility lacks unit-test parity with the main process; add a
Vitest test suite mirroring src/main/utils/__tests__/file.windows.test.ts that
covers the cases: "C:\\a\\b\\c.mp4" => ".mp4", "/a/b/c" => "", "/a/.env" => "",
"a.tar.gz" => ".gz", "a." => "", "" => "", and "C:/dir.with.dot/file" => "";
place tests under src/renderer/src/utils/__tests__/file.windows.test.ts, import
the renderer util, write assertions for each input->expected output, and run the
renderer test config to ensure they pass before opening the follow-up PR.

⚠️ Potential issue

Edge case: dotfiles treated as having an extension; diverges from Node/path semantics and main getFileExt.
For filenames like ".bashrc", this returns ".bashrc" (non-empty), whereas Node's path.extname returns "" and your main getFileExt intends that behavior. Align to ignore a leading dot with no further dots and avoid future main/renderer divergence.

Apply:

-  // 分割文件名以获取扩展名
-  const parts = fileName.split('.')
-  if (parts.length > 1) {
-    const extension = parts.slice(-1)[0].toLowerCase()
-
-    // 验证扩展名有效性:不能为空,不能包含路径分隔符
-    if (extension && !extension.includes('/') && !extension.includes('\\')) {
-      return '.' + extension
-    }
-  }
-
-  return ''
+  // 使用最后一个 '.' 获取扩展名;忽略以 '.' 开头且无后续点的隐藏文件
+  const lastDot = fileName.lastIndexOf('.')
+  if (lastDot > 0 && lastDot < fileName.length - 1) {
+    const ext = fileName.slice(lastDot + 1).toLowerCase()
+    if (ext && !/[\/\\]/.test(ext)) {
+      return '.' + ext.replace(/^\.+/, '') // '..mp4' -> '.mp4'
+    }
+  }
+  return ''
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!filePath) {
return ''
}
// 标准化路径分隔符和清理路径
let normalizedPath = filePath.trim()
// 处理反斜杠(Windows 路径)
normalizedPath = normalizedPath.replace(/\\/g, '/')
// 获取文件名部分(移除路径)
const fileName = normalizedPath.split('/').pop() || normalizedPath
// 分割文件名以获取扩展名
const parts = fileName.split('.')
if (parts.length > 1) {
const extension = parts.slice(-1)[0].toLowerCase()
return '.' + extension
// 验证扩展名有效性:不能为空,不能包含路径分隔符
if (extension && !extension.includes('/') && !extension.includes('\\')) {
return '.' + extension
}
}
return '.'
return ''
}
if (!filePath) {
return ''
}
// 标准化路径分隔符和清理路径
let normalizedPath = filePath.trim()
// 处理反斜杠(Windows 路径)
normalizedPath = normalizedPath.replace(/\\/g, '/')
// 获取文件名部分(移除路径)
const fileName = normalizedPath.split('/').pop() || normalizedPath
// 使用最后一个 '.' 获取扩展名;忽略以 '.' 开头且无后续点的隐藏文件
const lastDot = fileName.lastIndexOf('.')
if (lastDot > 0 && lastDot < fileName.length - 1) {
const ext = fileName.slice(lastDot + 1).toLowerCase()
if (ext && !/[\/\\]/.test(ext)) {
return '.' + ext.replace(/^\.+/, '') // '..mp4' -> '.mp4'
}
}
return ''
}
🤖 Prompt for AI Agents
In src/renderer/src/utils/file.ts around lines 20 to 45, the current logic
treats dotfiles like ".bashrc" as having an extension; update the extension
detection to match Node/path.extname and the main getFileExt by ignoring a
single leading dot with no other dots. Concretely: after computing fileName and
parts, if fileName startsWith('.') and parts.length === 2 then return '' (no
extension); otherwise proceed with the existing validation and return '.' +
extension when valid.

@mkdir700 mkdir700 deleted the branch alpha September 12, 2025 06:40
@mkdir700 mkdir700 closed this Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants